transform: optimize string / byte conversions#5492
Conversation
aykevl
left a comment
aykevl
left a comment
There was a problem hiding this comment.
This is a lot of code to review. And also, it doesn't include a lot of comments to explain why these transforms are safe and what they're even doing in the first place (although the names do help - sometimes, see below). Can you maybe look through the code to make it a bit more compact/readable?
| return | ||
| } | ||
|
|
||
| optimizeStringFromBytesStringEqual(mod, stringFromBytes) |
There was a problem hiding this comment.
Why do you put these in separate functions, instead of just writing the contents here?
| return false | ||
| } | ||
|
|
||
| func isSafeBetweenStringFromBytesUse(inst, stringFromBytes llvm.Value, allowedCalls map[llvm.Value]struct{}) bool { |
There was a problem hiding this comment.
Example: isSafeBetweenStringFromBytesUse ... what does that even mean? I would suggest renaming or at least explaining a bit more what it does in a comment.
| func optimizeStringFromBytesMapKey(mod llvm.Module, stringFromBytes llvm.Value) { | ||
| optimizeStringFromBytesMapKeyUses(mod.NamedFunction("runtime.hashmapStringGet"), stringFromBytes) | ||
| optimizeStringFromBytesMapKeyUses(mod.NamedFunction("runtime.hashmapStringDelete"), stringFromBytes) | ||
| } |
There was a problem hiding this comment.
Why not put it in a loop instead of a new function? I think a loop would make this more readable since it keeps all the relevant code together.
There was a problem hiding this comment.
Funnily it was all together and then I split it apart... I'll redo it
|
I meant to send this as a draft since I don't have a lot of free time and wasn't done 😅 |
|
If I can make a recommendation: usually these transforms are structured as follows:
That means having just a single optimization pass, otherwise if a Note that "safe uses" also means checking that every instruction between the |
5db2361 to
ed91ad3
Compare
Rewrite string equality comparisons when either operand comes from a temporary []byte-to-string conversion to compare the original slice data directly. Each comparison is optimized independently only when it is in the same basic block and every intervening instruction is known not to mutate the slice. The copied string remains for any other uses. Fixes issue 4045
Reuse the temporary []byte-to-string comparison rewrite for runtime.stringLess. The same-block safety check preserves conversions when intervening instructions could mutate the source slice.
Rewrite len(string(b)) to use the original slice length. This is always valid even when the copied string must remain for other uses.
|
Rewrote this and dropped out all of the followups. |
2 participants
The commits are clearer about this, and you can see the output changes as the tests are split apart.
Fixes #4045
bytes.Equalno longer allocates.